Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing error when insecure rng is used #294

Closed
wants to merge 1 commit into from

Conversation

uri
Copy link

@uri uri commented Oct 4, 2018

  • New option for v1 and v4: allowInsecureRng
    • Require to be true if rng.insecure == true

Closes #173


Here's my attempt at closing #173, please let me know if a different approach is preferred.

@broofa
Copy link
Member

broofa commented Oct 4, 2018

At this point I'm of the opinion this module shouldn't have an insecure option. Thus ...

  1. Get rid of the insecure and insecureMessage stuff
  2. Remove the checks in v1 and v4 (they're RNG agnostic and should stay that way)
  3. Change mathRNG() to noSecureRNG(), thusly:
function noSecureRNG() {
    throw Error('uuid: No secure RNG available. See <github link> for more info.');
}
  1. Add an rng option to v1, similar to v4, so users can pass a custom RNG if desired as a workaround to the error being thrown.

The link in step 3 should refer either to docs in the README or a separate wiki page in this project that describe:

  • Why the error occurs ("You're running on a JS platform that doesn't provide a secure RNG")
  • How to work around it ("Pass an options.rng param into either v1() or v4()`")
  • An example of how to do that.

That last point is a little problematic. Ideally we could recommend a cryptographically secure JS module upon which to build a CSPRNG function the reader could pass in, but that's easier said than done. Such modules tend to be either obscenely code-heavy or of questionable quality, and there's no obvious "right" choice. If we make a recommendation, we (meaning "I") end up dealing with any issues that arise, which I'm not interested in taking on. So... I think a simple example that just shows the basic mechism for passing in an rng function is sufficient. It could use an all-zeroes function as an example (function nilRNG() {return new Array(16).fill(0)}). What it should not do is provide an example that uses Math.random(), as that's just perpetuating the problem.

@broofa
Copy link
Member

broofa commented Oct 4, 2018

BTW, thanks for working on this! :-)

@uri
Copy link
Author

uri commented Oct 5, 2018

Thanks for the feedback. I'll make those requested changes.

@uri
Copy link
Author

uri commented Oct 9, 2018

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Re: the wiki, I'd actually forgotten about that page. :-) I think I wrote that for an aborted attempt at addressing this issue back in the day. Regardless, I've taken an editorial pass over it. 'Hope you don't mind.

Please change the commit message to conform to Conventional Commit guidelines. Specifically, change it to "BREAKING CHANGE: Removed Math.random rng fallback"


return rnds;
module.exports = function noSecureRNG() {
throw Error('uuid: No secure RNG available. See https://github.com/kelektiv/node-uuid/wiki#no-good-rng-error for more info.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lime
Copy link

lime commented Apr 14, 2019

Hi! I was wondering if I can be of any help in getting this across the finish line?

It seems like the only things missing are an amended commit message and a changed wiki link. I forked this branch and made those changes here: https://github.com/lime/node-uuid/tree/error-on-insecure-rng

If you want, I could open a separate pull request from it. You're also welcome to pluck those commits in directly. I just wanted to check first whether it would be welcomed, and if there's anything else that still needs work.

Edit: and obviously, if the original author has the time to make the changes themselves, by all means. :) Just thought I'd help if I can.

@uri
Copy link
Author

uri commented Apr 17, 2019

@broofa I made the change to the commit message is there anything else?

@ctavan ctavan mentioned this pull request Jan 23, 2020
ctavan added a commit that referenced this pull request Jan 27, 2020
Remove insecure builtin random number generator (RNG)

Closes #294 and fixes #173
@ctavan
Copy link
Member

ctavan commented Jan 27, 2020

Closed through #354

@ctavan ctavan closed this Jan 27, 2020
@ctavan ctavan mentioned this pull request Feb 24, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insecure random values should be opt-in
4 participants